Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Window Resizing Edge Case #3345

Merged
merged 3 commits into from
Dec 13, 2022

Conversation

daniellerozenblit
Copy link
Contributor

This PR fixes the bug addressed in #3239.

I have implemented the fix proposed and have validated that the issue no longer occurs.

dd if=/dev/zero of=testFile  bs=1024M  count=1

./zstd --long=31 -1 --single-thread --no-content-size -f ~/testFile
~/testFile :  0.00%   ( 1.000 GiB =>   32.0 KiB, /home/drozenblit/testFile.zst) 

./zstd -l -v ~/testFile.zst
Window Size: 1.000 GiB (1073741824 B)

@Cyan4973
Copy link
Contributor

Would it be possible to add an associated test in playTests.sh ?

@terrelln
Copy link
Contributor

I'd recommend adding a test to cli-tests compression tests, should be a bit easier than in playTests.sh.

https://github.com/facebook/zstd/tree/dev/tests/cli-tests/compression
https://github.com/facebook/zstd/tree/dev/tests/cli-tests/README.md

zstd --long=31 -1 --single-thread --no-content-size -f file
zstd -l -v file.zst

# We want to ignore stderr (its outputting "*** zstd command line interface
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One way to silence stderr could be 2> /dev/null (redirects stderr to /dev/null)

@@ -0,0 +1 @@
...
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor : I'm wondering what happens if this file is not provided, or if it is replaced by the *.ignore suffix variant.
This could be a more explicit way to express that stderr output is not significant.
cc @terrelln .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the .ignore suffix seems to work! I can switch to that, I like that it is a bit more explicit than redirection.

@daniellerozenblit daniellerozenblit marked this pull request as ready for review December 13, 2022 22:56
@daniellerozenblit daniellerozenblit merged commit b0bcbbf into facebook:dev Dec 13, 2022
@daniellerozenblit daniellerozenblit deleted the fix-1GiB-file-bug branch December 15, 2022 21:06
@Cyan4973 Cyan4973 mentioned this pull request Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants